Skip to content

Implements Sum,sum_checked,min,max,is Distict,inverse for REE. - #7933

Closed
ghost wants to merge 3 commits into
mainfrom
unknown repository
Closed

Implements Sum,sum_checked,min,max,is Distict,inverse for REE. #7933
ghost wants to merge 3 commits into
mainfrom
unknown repository

Conversation

@ghost

@ghost ghost commented Jul 15, 2025 •
edited by ghost
Loading

Copy link
Copy Markdown

Which issue does this PR close?

This PR works towards closing the larger REE Epic

Rationale for this change

Add operations onto the REE datatype such as

  • Sum
  • Sum_checked
  • IS DISTINCT FROM
  • Max/Min
  • Distinct (Arrow already handles this apparently)

What changes are included in this PR?

Allows for REE columns to be used for the previously mentioned functions correctly and efficently.

Are these changes tested?

Yes, comprehensive tests have been added in arrow-ord/src/cmp.rs and arrow-arith/src/aggregate.rs:

  • Basic functionality tests: Same values, different values, mixed scenarios
  • Edge case tests: Empty arrays, single runs, all nulls, mixed nulls and values
  • Data type tests: Float64 and Timestamp types
  • Both operations: distinct and not_distinct operations
  • The tests verify that REE distinct operations work correctly without array expansion and handle null values properly.

Are there any user-facing changes?

Performance improvement: REE distinct operations are now much faster for datasets with repeated values
No API changes: Existing distinct() and not_distinct() functions work the same way but are now more efficient for REE arrays
No breaking changes: All existing functionality is preserved

If there are user-facing changes then we may require documentation to be updated before approving the PR.

If there are any breaking changes to public APIs, please call them out.

@Rich-T-kid
implements Sum,sum_checked,min,max,is Distict,inverse for REE. ALso i…
acb5d08 
…ncludes helper functions for expanding REE into logical represention
@github-actions github-actions Bot added the arrow Changes to the arrow crate label Jul 15, 2025
Rich-T-kid
Rich-T-kid reviewed Jul 18, 2025
View reviewed changes
Comment thread arrow-arith/src/aggregate.rs Outdated
Rich-T-kid
Rich-T-kid reviewed Jul 18, 2025
View reviewed changes
Comment thread arrow-ord/src/cmp.rs Outdated
Rich-T-kid
Rich-T-kid reviewed Jul 18, 2025
View reviewed changes
Comment thread arrow-ord/src/cmp.rs Outdated
Rich-T-kid
Rich-T-kid reviewed Jul 18, 2025
View reviewed changes
Comment thread arrow-ord/src/cmp.rs Outdated
gabotechs
gabotechs reviewed Jul 22, 2025
View reviewed changes
Comment thread arrow-arith/src/aggregate.rs Outdated
Comment thread arrow-arith/src/aggregate.rs Outdated
Comment thread arrow-arith/src/aggregate.rs Outdated
Comment thread arrow-array/src/array/mod.rs Outdated
Comment thread arrow-array/src/array/run_array.rs
Comment thread arrow-array/src/array/run_array.rs Outdated
Comment thread arrow-array/src/array/run_array.rs Outdated
Comment thread arrow-ord/src/cmp.rs Outdated
Comment thread arrow-ord/src/cmp.rs Outdated
Comment thread arrow-ord/src/lib.rs Outdated
@ghost ghost changed the title [Draft] implements Sum,sum_checked,min,max,is Distict,inverse for REE. Implements Sum,sum_checked,min,max,is Distict,inverse for REE. Jul 24, 2025
@ghost ghost mentioned this pull request Jul 24, 2025
Closed
24 tasks
@ghost
ghost marked this pull request as ready for review July 25, 2025 13:54
@Jefffrey

Jefffrey commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

Just want to checkup on this PR, if it's still being worked on? Looking to see if we can get REE support pushed along in arrow-rs

@brunal

brunal commented Feb 8, 2026

Copy link
Copy Markdown
Contributor

According to #3520 (comment), the work was done while @rich-t-kid-datadog was an intern, and he's not available anymore. I can pick it up -- which should mostly be resolving merge conflicts.

@Jefffrey

Jefffrey commented Feb 9, 2026

Copy link
Copy Markdown
Contributor

That sounds good, I'm willing to help review this to get it along

@brunal

brunal commented Feb 10, 2026

Copy link
Copy Markdown
Contributor

This isn't ready as-is:

  • aggregate works, but is inefficient for min-max: it goes through all values (cost O(n*log(n)) for n values), when it could just look at the run-ends child array (cost O(# runs) < O(n))
  • cmp only implements distinct, and is inefficient as well (binary searches for logical->physical index)

I'll send a first PR just for aggregate, and later a second PR for cmp.

@brunal

brunal commented Feb 13, 2026

Copy link
Copy Markdown
Contributor

Aggregate work done in #9409. In particular, it takes care of sliced Ree arrays.

@brunal

brunal commented Feb 21, 2026

Copy link
Copy Markdown
Contributor

CMP done in #9448

@alamb

alamb commented Mar 18, 2026

Copy link
Copy Markdown
Contributor

Marking as draft as I think this PR is no longer waiting on feedback and I am trying to make it easier to find PRs in need of review. Please mark it as ready for review when it is ready for another look

@alamb
alamb marked this pull request as draft March 18, 2026 14:16
@ghost ghost closed this by deleting the head repository Mar 25, 2026
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

arrow Changes to the arrow crate

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@Jefffrey @brunal @alamb @gabotechs @Rich-T-kid